feat(clp-s): Add CLP_S_DISABLE_CURL build option to exclude libcurl/libcrypto paths for WebAssembly compatibility; Improve Curl error handling utilities.#1710
Open
Bill-hbrhbr wants to merge 39 commits intoy-scope:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces optional exclusion of libcurl from CLP_S builds via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bill-hbrhbr
commented
Dec 2, 2025
Bill-hbrhbr
changed the title
glibc and libm to improve portability.
Bill-hbrhbr
changed the title
glibc and libm to improve portability.glibc and libm to improve portability; Update mongocxx to link against statically built zstd.
LinZhihao-723
requested changes
Dec 2, 2025
| : ::clp_s::search::OutputHandler(should_output_timestamp, true), | ||
| m_batch_size(batch_size), | ||
| m_max_num_results(max_num_results) { | ||
| #if CLP_S_STATIC_EXE |
Member
There was a problem hiding this comment.
Before we move forward: we should use the macro to disable the entire function if this whole function should be enabled/disabled. For example:
#if CLP_S_STATIC_EXE
auto foo() -> void { throw ... }
#else
auto foo() -> void { /*actual implementation*/ }
#endifAlso, I think it's worth to derive a dedicated exception (ideally from ystdlib) for disabled features.
Contributor
Author
There was a problem hiding this comment.
Is the dedicated exception gonna be based on TraceableException?
gibber9809
requested changes
Dec 2, 2025
Contributor
gibber9809
left a comment
gibber9809
left a comment
There was a problem hiding this comment.
Besides the comment about the flags I think we also need to hide all of the options that don't work from CommandLineArguments.cpp, since it's pretty confusing for the binary to display a bunch of options that don't work in the help text.
That means hiding:
- All of the
authoptions - All of the mongo db related options/the entire sections they are in
- All of the examples/help text referencing these options
As well, I'm wondering why we have to exclude libcurl? If it's just too much work to get the linking working in the short term that's fine I guess, but it is a pretty significant limitation, so we should try to fix it if possible.
I'm also not totally sure about the style for how we're excluding code using macros. Will give it some more thought for the next round of review.
Member
@gibber9809 In terms of excluding Yeah it's the linking issue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/core/src/clp_s/OutputHandlerImpl.cpp (1)
68-174: Consider using a dedicated exception for disabled features.The past review discussion suggested deriving a dedicated exception (ideally from
ystdlib) for disabled features rather than excluding compilation entirely. While the current approach of conditionally compiling out the entire implementation is valid, a dedicated exception would provide clearer runtime error messages when users attempt to use functionality that was excluded from the build.For example:
#if CLP_S_EXCLUDE_MONGOCXX ResultsCacheOutputHandler::ResultsCacheOutputHandler(...) { throw OperationNotSupported(ErrorCodeFeatureDisabled, __FILENAME__, __LINE__, "MongoDB support excluded from this build"); } #else // existing implementation #endifBased on learnings, this pattern would better communicate build-time exclusions to users and align with the previous review discussion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
components/core/src/clp_s/CMakeLists.txt(6 hunks)components/core/src/clp_s/InputConfig.cpp(3 hunks)components/core/src/clp_s/JsonConstructor.cpp(5 hunks)components/core/src/clp_s/JsonParser.cpp(3 hunks)components/core/src/clp_s/OutputHandlerImpl.cpp(2 hunks)components/core/src/clp_s/OutputHandlerImpl.hpp(2 hunks)components/core/src/clp_s/clp-s.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp_s/JsonParser.cppcomponents/core/src/clp_s/clp-s.cppcomponents/core/src/clp_s/JsonConstructor.cppcomponents/core/src/clp_s/InputConfig.cppcomponents/core/src/clp_s/OutputHandlerImpl.cppcomponents/core/src/clp_s/OutputHandlerImpl.hpp
🧠 Learnings (28)
📓 Common learnings
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1229
File: taskfiles/deps/main.yaml:494-504
Timestamp: 2025-08-19T07:08:15.583Z
Learning: In zlib v1.3.1's CMake build system, only ZLIB_BUILD_EXAMPLES is a valid option. Other ZLIB_BUILD_* flags (MINIZIP, SHARED, STATIC, TESTING) and ZLIB_INSTALL are not defined in the CMakeLists.txt and have no effect. Use BUILD_SHARED_LIBS=ON for shared library control and ZLIB_BUILD_EXAMPLES=OFF to disable examples.
Learnt from: sitaowang1998
Repo: y-scope/clp PR: 1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
📚 Learning: 2024-10-07T21:16:41.660Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:769-779
Timestamp: 2024-10-07T21:16:41.660Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, when handling errors in `parse_from_ir`, prefer to maintain the current mix of try-catch and if-statements because specific messages are returned back up in some cases.
Applied to files:
components/core/src/clp_s/JsonParser.cppcomponents/core/src/clp_s/clp-s.cppcomponents/core/src/clp_s/JsonConstructor.cpp
📚 Learning: 2024-10-07T21:35:04.362Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:735-794
Timestamp: 2024-10-07T21:35:04.362Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `parse_from_ir` method, encountering errors from `kv_log_event_result.error()` aside from `std::errc::no_message_available` and `std::errc::result_out_of_range` is anticipated behavior and does not require additional error handling or logging.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-11-15T03:15:45.919Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 593
File: components/core/tests/test-NetworkReader.cpp:216-219
Timestamp: 2024-11-15T03:15:45.919Z
Learning: In the `network_reader_with_valid_http_header_kv_pairs` test case in `components/core/tests/test-NetworkReader.cpp`, additional error handling for JSON parsing failures is not necessary, as the current error message is considered sufficient.
Applied to files:
components/core/src/clp_s/JsonParser.cppcomponents/core/src/clp_s/InputConfig.cpp
📚 Learning: 2024-10-07T21:38:35.979Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:581-627
Timestamp: 2024-10-07T21:38:35.979Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `get_archive_node_id` method, throwing a string literal as an exception is acceptable practice.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-12-10T16:03:13.322Z
Learnt from: gibber9809
Repo: y-scope/clp PR: 630
File: components/core/src/clp_s/JsonParser.cpp:702-703
Timestamp: 2024-12-10T16:03:13.322Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, validation and exception throwing are unnecessary in the `get_archive_node_id` method when processing nodes, and should not be added.
Applied to files:
components/core/src/clp_s/JsonParser.cppcomponents/core/src/clp_s/JsonConstructor.cpp
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:629-733
Timestamp: 2024-10-08T15:52:50.753Z
Learning: In the `JsonParser::parse_kv_log_event` method in `components/core/src/clp_s/JsonParser.cpp`, prefer passing exceptions to higher levels for more graceful handling, rather than handling them at that level.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-12-10T16:03:08.691Z
Learnt from: gibber9809
Repo: y-scope/clp PR: 630
File: components/core/src/clp_s/JsonParser.cpp:702-703
Timestamp: 2024-12-10T16:03:08.691Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `get_archive_node_id` function, validation and exception throwing for UTF-8 compliance of `curr_node.get_key_name()` are unnecessary and should be omitted.
Applied to files:
components/core/src/clp_s/JsonParser.cppcomponents/core/src/clp_s/JsonConstructor.cpp
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:756-765
Timestamp: 2024-10-08T15:52:50.753Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `parse_from_ir()` function, reaching the end of log events in a given IR is not considered an error case. The errors `std::errc::no_message_available` and `std::errc::result_out_of_range` are expected signals to break the deserialization loop and proceed accordingly.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-10-24T14:45:26.265Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 523
File: components/core/src/clp/BufferedFileReader.cpp:96-106
Timestamp: 2024-10-24T14:45:26.265Z
Learning: In `components/core/src/clp/BufferedFileReader.cpp`, refactoring the nested error handling conditions may not apply due to the specific logic in the original code.
Applied to files:
components/core/src/clp_s/JsonParser.cppcomponents/core/src/clp_s/clp-s.cppcomponents/core/src/clp_s/InputConfig.cpp
📚 Learning: 2025-01-30T19:26:33.869Z
Learnt from: davemarco
Repo: y-scope/clp PR: 700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.hpp:153-155
Timestamp: 2025-01-30T19:26:33.869Z
Learning: When working with constexpr strings (string literals with static storage duration), std::string_view is the preferred choice for member variables as it's more efficient and safe, avoiding unnecessary memory allocations.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/clp-s.cpp:256-258
Timestamp: 2024-10-08T15:52:50.753Z
Learning: Error handling for file path validation is performed in the `JsonParser` constructor.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-11-01T03:26:26.386Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Applied to files:
components/core/src/clp_s/clp-s.cpp
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/src/clp_s/clp-s.cppcomponents/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/OutputHandlerImpl.cpp
📚 Learning: 2025-07-01T14:49:07.333Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.
Applied to files:
components/core/src/clp_s/clp-s.cpp
📚 Learning: 2025-06-02T18:22:24.060Z
Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
Repo: y-scope/clp PR: 1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-06T07:32:28.462Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-19T07:08:15.583Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1229
File: taskfiles/deps/main.yaml:494-504
Timestamp: 2025-08-19T07:08:15.583Z
Learning: In zlib v1.3.1's CMake build system, only ZLIB_BUILD_EXAMPLES is a valid option. Other ZLIB_BUILD_* flags (MINIZIP, SHARED, STATIC, TESTING) and ZLIB_INSTALL are not defined in the CMakeLists.txt and have no effect. Use BUILD_SHARED_LIBS=ON for shared library control and ZLIB_BUILD_EXAMPLES=OFF to disable examples.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-09-03T07:29:35.223Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1107
File: components/core/src/clp_s/CMakeLists.txt:106-116
Timestamp: 2025-09-03T07:29:35.223Z
Learning: In the CLP project, do not suggest adding CONFIG or MODULE specifications to find_package calls. The maintainers prefer using find_package without these mode specifications.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-25T07:27:19.449Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1262
File: docs/src/dev-guide/components-core/index.md:50-50
Timestamp: 2025-08-25T07:27:19.449Z
Learning: In the CLP project, nlohmann_json version 3.11.3 is installed through their dependency management process, but they don't enforce version constraints in CMake find_package calls - they control versions through installation rather than CMake version pinning.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-01-29T22:16:52.665Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.cpp:29-46
Timestamp: 2025-01-29T22:16:52.665Z
Learning: In C++ code, when throwing exceptions, prefer throwing the original return code (RC) from lower-level operations instead of a generic error code to preserve error context and aid in debugging.
Applied to files:
components/core/src/clp_s/OutputHandlerImpl.cpp
🧬 Code graph analysis (1)
components/core/src/clp_s/JsonConstructor.cpp (1)
components/core/src/clp_s/JsonConstructor.hpp (1)
OperationFailed(39-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: package-image
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: rust-checks
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (6)
components/core/src/clp_s/JsonConstructor.cpp (1)
83-196: LGTM! Clean conditional compilation of MongoDB functionality.The MongoDB-related code paths are well-organized with clear conditional compilation guards. The approach ensures that:
- Client and collection initialization (lines 84-95) are excluded when
CLP_S_EXCLUDE_MONGOCXXis defined- Result vector and document building (lines 97, 114-139) are excluded consistently
- The final bulk insert (lines 189-195) is excluded
The code maintains logical flow in both build configurations without introducing runtime errors or leaving dangling references.
components/core/src/clp_s/OutputHandlerImpl.hpp (1)
118-202: LGTM! Consistent conditional compilation of MongoDB output handler.The
ResultsCacheOutputHandlerclass declaration and all its MongoDB-dependent members are properly guarded with#if !CLP_S_EXCLUDE_MONGOCXX. This is consistent with the implementation in OutputHandlerImpl.cpp and ensures clean compilation when MongoDB support is excluded.The guard placement correctly includes:
- The entire class declaration (lines 122-201)
- All MongoDB-specific members (
m_client,m_collection,m_results, etc.)- Helper structs (
QueryResult,QueryResultGreaterTimestampComparator)components/core/src/clp_s/CMakeLists.txt (3)
1-31: LGTM! Well-structured build configuration with dependent options.The introduction of
CLP_S_BUILD_SIMPLIFIED_STATIC_EXECUTABLEas a top-level option with three dependent sub-options is a clean design that addresses the past review feedback about splitting functionality into separate flags.Key strengths:
- Uses
cmake_dependent_optioncorrectly to cascade options- Clear naming that describes what each option does
- Options default to OFF when parent option is disabled
- Each sub-option can be independently controlled when the parent is enabled
This structure makes it easy for users to:
- Enable the simplified static build with one flag
- Selectively override individual exclusions if needed
- Understand what each option controls
Based on learnings, this approach aligns with CLP's preference for flexible build configurations.
508-515: LGTM! Correct static runtime linking for the executable.The conditional static linking of libgcc and libstdc++ is correctly applied only to the clp-s executable (not libraries), which is the appropriate target for static runtime linking. The flags
-static-libgccand-static-libstdc++will reduce runtime dependencies as intended for the pip distribution use case.Note: This assumes the target system has a compatible glibc version, as mentioned in the PR objectives. Users should be aware that while this reduces dependencies, glibc and libm will still be dynamically linked.
245-249: Verify flag propagation to all components that use them.The compile definitions are propagated to several components, but verify that all components using these preprocessor guards receive the appropriate definitions:
Components receiving
CLP_S_EXCLUDE_LIBCURL:
- clp_s_io (line 248)
- clp_s_archive_writer (line 314)
- clp-s executable (line 481)
Components receiving
CLP_S_EXCLUDE_MONGOCXX:
- clp_s_json_constructor (line 418)
- clp-s executable (line 482)
Components NOT receiving flags but that contain conditional code:
- OutputHandlerImpl.cpp is part of CLP_S_EXE_SOURCES, so it gets flags via clp-s executable ✓
Run the following to verify all files using these guards are part of components receiving the definitions:
#!/bin/bash # Find files using CLP_S_EXCLUDE_LIBCURL echo "Files using CLP_S_EXCLUDE_LIBCURL:" rg -l "CLP_S_EXCLUDE_LIBCURL" components/core/src/clp_s/ --type cpp echo -e "\nFiles using CLP_S_EXCLUDE_MONGOCXX:" rg -l "CLP_S_EXCLUDE_MONGOCXX" components/core/src/clp_s/ --type cpp echo -e "\nVerify these files are part of the targets receiving compile definitions:" echo "- clp_s_io (CLP_S_EXCLUDE_LIBCURL)" echo "- clp_s_archive_writer (CLP_S_EXCLUDE_LIBCURL)" echo "- clp_s_json_constructor (CLP_S_EXCLUDE_MONGOCXX)" echo "- clp-s executable (both flags)"Also applies to: 311-315, 415-419, 478-483
components/core/src/clp_s/clp-s.cpp (1)
304-309: Address negation operator in preprocessor conditionals.The code uses
#if !CLP_S_EXCLUDE_MONGOCXXand#if !CLP_S_EXCLUDE_LIBCURL. If the coding guideline requiringfalse == <expression>over!<expression>applies to preprocessor directives, these should be rewritten as#if false == CLP_S_EXCLUDE_MONGOCXXand#if false == CLP_S_EXCLUDE_LIBCURLfor consistency. Verify the guideline applicability to preprocessor conditionals and confirm initialization order dependencies between mongocxx and curl instances.
components/core/src/clp_s/clp-s.cpp
Outdated
Comment on lines
258
to
268
| #if CLP_S_EXCLUDE_MONGOCXX | ||
| throw std::runtime_error("Simplified static clp-s executable does not support mongocxx."); | ||
| #else | ||
| output_handler = std::make_unique<clp_s::ResultsCacheOutputHandler>( | ||
| command_line_arguments.get_mongodb_uri(), | ||
| command_line_arguments.get_mongodb_collection(), | ||
| command_line_arguments.get_batch_size(), | ||
| command_line_arguments.get_max_num_results() | ||
| ); | ||
| break; | ||
| #endif |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a dedicated exception type for excluded features.
Similar to the comment on OutputHandlerImpl.cpp, the runtime_error thrown at line 259 when CLP_S_EXCLUDE_MONGOCXX is defined could be replaced with a dedicated exception type that better communicates build-time feature exclusions.
Since this exception is caught at line 276 and logged via SPDLOG_ERROR, using a more specific exception type (e.g., FeatureNotAvailableException) would:
- Allow callers to distinguish between build-time exclusions and runtime errors
- Provide clearer error messages
- Maintain consistency with the codebase's exception handling patterns
This aligns with the earlier review discussion about dedicated exceptions for disabled features.
Contributor
Author
There was a problem hiding this comment.
We plan to add a dedicated exception type later in ystdlib-cpp
Contributor
There was a problem hiding this comment.
@Bill-hbrhbr, sounds good! Adding a dedicated exception type in ystdlib-cpp makes sense for consistent handling of build-time feature exclusions across the codebase. Looking forward to seeing that improvement in a future PR.
🧠 Learnings used
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:581-627
Timestamp: 2024-10-08T15:52:50.753Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `get_archive_node_id` method, throwing a string literal as an exception is acceptable practice.
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:581-627
Timestamp: 2024-10-07T21:38:35.979Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `get_archive_node_id` method, throwing a string literal as an exception is acceptable practice.
Learnt from: AVMatthews
Repo: y-scope/clp PR: 595
File: components/core/tests/test-clp_s-end_to_end.cpp:141-142
Timestamp: 2024-11-29T22:51:00.355Z
Learning: In 'components/core/tests/test-clp_s-end_to_end.cpp', using `WIFEXITED` and `WEXITSTATUS` with the return value of `std::system` is acceptable for checking the exit status of system commands, as per the C++ documentation.
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:769-779
Timestamp: 2024-10-07T21:16:41.660Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, when handling errors in `parse_from_ir`, prefer to maintain the current mix of try-catch and if-statements because specific messages are returned back up in some cases.
Learnt from: haiqi96
Repo: y-scope/clp PR: 700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.cpp:29-46
Timestamp: 2025-01-29T22:16:52.665Z
Learning: In C++ code, when throwing exceptions, prefer throwing the original return code (RC) from lower-level operations instead of a generic error code to preserve error context and aid in debugging.
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 549
File: components/core/src/clp/ffi/ir_stream/Deserializer.hpp:209-210
Timestamp: 2024-10-08T15:52:50.753Z
Learning: Until #486 is merged with customized error code support, introducing a layer from `IRErrorCode` to `std::errc` is unnecessary.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/core/src/clp_s/JsonParser.cpp (1)
17-17: Guard thecurl.hinclude to support builds without curl headers.The curl include is only required when
CLP_S_EXCLUDE_LIBCURLis not defined (curl types are used at line 1340 in the non-excluded implementation). Consider guarding it:+#if !CLP_S_EXCLUDE_LIBCURL #include <curl/curl.h> +#endifThis prevents compilation issues in environments where curl headers are unavailable and makes the dependency explicit.
♻️ Duplicate comments (1)
components/core/src/clp_s/JsonParser.cpp (1)
1319-1330: Critical: Replace exception throw with boolean return to match function contract.The excluded-libcurl implementation throws an exception when a
NetworkReaderis detected, which is inconsistent with:
- The function signature (returns
boolto indicate error status)- The non-excluded implementation (returns
trueon error,falseotherwise)- The usage at line 659, which expects a boolean return value to determine error handling
Throwing an exception will bypass the graceful error handling at line 659 and propagate unexpectedly. The function should consistently return a boolean across both implementations.
Apply this diff to match the non-excluded implementation's contract:
#if CLP_S_EXCLUDE_LIBCURL bool JsonParser::check_and_log_curl_error( Path const& path, std::shared_ptr<clp::ReaderInterface> reader ) { if (auto network_reader = std::dynamic_pointer_cast<clp::NetworkReader>(reader); nullptr != network_reader) { - throw std::runtime_error("Simplified static clp-s executable does not support libcurl."); + SPDLOG_ERROR( + "Simplified static clp-s executable does not support network ingestion for input {}", + path.path + ); + return true; } return false; }This logs a clear error message and returns
trueto indicate an error occurred, consistent with the non-excluded implementation's behaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/core/src/clp_s/JsonParser.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit configuration file
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp_s/JsonParser.cpp
🧠 Learnings (14)
📓 Common learnings
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Learnt from: sitaowang1998
Repo: y-scope/clp PR: 1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1229
File: taskfiles/deps/main.yaml:494-504
Timestamp: 2025-08-19T07:08:15.583Z
Learning: In zlib v1.3.1's CMake build system, only ZLIB_BUILD_EXAMPLES is a valid option. Other ZLIB_BUILD_* flags (MINIZIP, SHARED, STATIC, TESTING) and ZLIB_INSTALL are not defined in the CMakeLists.txt and have no effect. Use BUILD_SHARED_LIBS=ON for shared library control and ZLIB_BUILD_EXAMPLES=OFF to disable examples.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1226
File: taskfiles/deps/main.yaml:522-527
Timestamp: 2025-09-15T18:53:15.844Z
Learning: When ystdlib is built with -Dystdlib_BUILD_TESTING=OFF in taskfiles/deps/main.yaml, it does not require Catch2 settings to be injected via CMAKE_GEN_ARGS since testing is disabled and Catch2 is not needed during the build process.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
📚 Learning: 2024-10-07T21:16:41.660Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:769-779
Timestamp: 2024-10-07T21:16:41.660Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, when handling errors in `parse_from_ir`, prefer to maintain the current mix of try-catch and if-statements because specific messages are returned back up in some cases.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-10-07T21:35:04.362Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:735-794
Timestamp: 2024-10-07T21:35:04.362Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `parse_from_ir` method, encountering errors from `kv_log_event_result.error()` aside from `std::errc::no_message_available` and `std::errc::result_out_of_range` is anticipated behavior and does not require additional error handling or logging.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-11-15T03:15:45.919Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 593
File: components/core/tests/test-NetworkReader.cpp:216-219
Timestamp: 2024-11-15T03:15:45.919Z
Learning: In the `network_reader_with_valid_http_header_kv_pairs` test case in `components/core/tests/test-NetworkReader.cpp`, additional error handling for JSON parsing failures is not necessary, as the current error message is considered sufficient.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-10-07T21:38:35.979Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:581-627
Timestamp: 2024-10-07T21:38:35.979Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `get_archive_node_id` method, throwing a string literal as an exception is acceptable practice.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-12-10T16:03:13.322Z
Learnt from: gibber9809
Repo: y-scope/clp PR: 630
File: components/core/src/clp_s/JsonParser.cpp:702-703
Timestamp: 2024-12-10T16:03:13.322Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, validation and exception throwing are unnecessary in the `get_archive_node_id` method when processing nodes, and should not be added.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-12-10T16:03:08.691Z
Learnt from: gibber9809
Repo: y-scope/clp PR: 630
File: components/core/src/clp_s/JsonParser.cpp:702-703
Timestamp: 2024-12-10T16:03:08.691Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `get_archive_node_id` function, validation and exception throwing for UTF-8 compliance of `curr_node.get_key_name()` are unnecessary and should be omitted.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:629-733
Timestamp: 2024-10-08T15:52:50.753Z
Learning: In the `JsonParser::parse_kv_log_event` method in `components/core/src/clp_s/JsonParser.cpp`, prefer passing exceptions to higher levels for more graceful handling, rather than handling them at that level.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/JsonParser.cpp:756-765
Timestamp: 2024-10-08T15:52:50.753Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, within the `parse_from_ir()` function, reaching the end of log events in a given IR is not considered an error case. The errors `std::errc::no_message_available` and `std::errc::result_out_of_range` are expected signals to break the deserialization loop and proceed accordingly.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-10-14T03:42:10.355Z
Learnt from: LinZhihao-723
Repo: y-scope/clp PR: 558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-10-24T14:45:26.265Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 523
File: components/core/src/clp/BufferedFileReader.cpp:96-106
Timestamp: 2024-10-24T14:45:26.265Z
Learning: In `components/core/src/clp/BufferedFileReader.cpp`, refactoring the nested error handling conditions may not apply due to the specific logic in the original code.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2025-01-29T22:16:52.665Z
Learnt from: haiqi96
Repo: y-scope/clp PR: 700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.cpp:29-46
Timestamp: 2025-01-29T22:16:52.665Z
Learning: In C++ code, when throwing exceptions, prefer throwing the original return code (RC) from lower-level operations instead of a generic error code to preserve error context and aid in debugging.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2025-01-30T19:26:33.869Z
Learnt from: davemarco
Repo: y-scope/clp PR: 700
File: components/core/src/clp/streaming_archive/ArchiveMetadata.hpp:153-155
Timestamp: 2025-01-30T19:26:33.869Z
Learning: When working with constexpr strings (string literals with static storage duration), std::string_view is the preferred choice for member variables as it's more efficient and safe, avoiding unnecessary memory allocations.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
📚 Learning: 2024-10-08T15:52:50.753Z
Learnt from: AVMatthews
Repo: y-scope/clp PR: 543
File: components/core/src/clp_s/clp-s.cpp:256-258
Timestamp: 2024-10-08T15:52:50.753Z
Learning: Error handling for file path validation is performed in the `JsonParser` constructor.
Applied to files:
components/core/src/clp_s/JsonParser.cpp
🧬 Code graph analysis (1)
components/core/src/clp_s/JsonParser.cpp (1)
components/core/src/clp_s/JsonParser.hpp (3)
path(222-222)reader(82-86)reader(96-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: package-image
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: rust-checks
- GitHub Check: lint-check (ubuntu-24.04)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/core/src/clp_s/CMakeLists.txt (1)
392-423: Add missing compile definition toclp_s_archive_reader.Line 398 defines
clp_s_archive_reader, but unlikeclp_s_archive_writer(line 323) andclp_s_io(line 257), it does not expose theCLP_S_EXCLUDE_LIBCURLcompile definition. However, lines 416–422 conditionally link CURL based on this flag. Without the compile definition, source files inclp_s_archive_readercannot guard against CURL-dependent code at compile time, creating inconsistency and a potential correctness gap.Apply this diff to add the compile definition:
if(CLP_BUILD_CLP_S_ARCHIVEREADER) add_library( clp_s_archive_reader ${CLP_S_ARCHIVE_READER_SOURCES} ) add_library(clp_s::archive_reader ALIAS clp_s_archive_reader) + target_compile_definitions( + clp_s_archive_reader + PRIVATE + CLP_S_EXCLUDE_LIBCURL=$<BOOL:${CLP_S_EXCLUDE_LIBCURL}> + ) target_compile_features(clp_s_archive_reader PRIVATE cxx_std_20)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/src/clp_s/CMakeLists.txt(9 hunks)taskfile.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1226
File: taskfiles/deps/main.yaml:522-527
Timestamp: 2025-09-15T18:53:15.844Z
Learning: When ystdlib is built with -Dystdlib_BUILD_TESTING=OFF in taskfiles/deps/main.yaml, it does not require Catch2 settings to be injected via CMAKE_GEN_ARGS since testing is disabled and Catch2 is not needed during the build process.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Learnt from: sitaowang1998
Repo: y-scope/clp PR: 1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
taskfile.yamlcomponents/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-05-28T18:33:30.155Z
Learnt from: anlowee
Repo: y-scope/clp PR: 925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
Applied to files:
taskfile.yaml
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
taskfile.yaml
📚 Learning: 2025-10-22T21:01:31.391Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:01:31.391Z
Learning: In y-scope/clp, for the clp-rust-checks workflow (GitHub Actions) and the Taskfile target deps:lock:check-rust, we should verify Rust Cargo.lock is in sync with Cargo.toml using a non-mutating method (e.g., cargo metadata --locked / cargo check --locked) to keep CI deterministic and avoid updating dependencies during validation.
Applied to files:
taskfile.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-06-02T18:22:24.060Z
Learnt from: gibber9809
Repo: y-scope/clp PR: 955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-03T18:56:07.366Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:49-65
Timestamp: 2025-08-03T18:56:07.366Z
Learning: In CLP's custom CMake Find modules, `FindStaticLibraryDependencies` populates the `*_DYNAMIC_LIBS` variable even when building with static libraries. When the main library is static, all dependencies must also use static libraries - there's no fallback to shared libraries for dependencies.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-06-27T01:49:15.724Z
Learnt from: sitaowang1998
Repo: y-scope/clp PR: 1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-13T19:58:26.058Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.058Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-19T07:08:15.583Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1229
File: taskfiles/deps/main.yaml:494-504
Timestamp: 2025-08-19T07:08:15.583Z
Learning: In zlib v1.3.1's CMake build system, only ZLIB_BUILD_EXAMPLES is a valid option. Other ZLIB_BUILD_* flags (MINIZIP, SHARED, STATIC, TESTING) and ZLIB_INSTALL are not defined in the CMakeLists.txt and have no effect. Use BUILD_SHARED_LIBS=ON for shared library control and ZLIB_BUILD_EXAMPLES=OFF to disable examples.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-06T07:32:28.462Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-09-03T07:29:35.223Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1107
File: components/core/src/clp_s/CMakeLists.txt:106-116
Timestamp: 2025-09-03T07:29:35.223Z
Learning: In the CLP project, do not suggest adding CONFIG or MODULE specifications to find_package calls. The maintainers prefer using find_package without these mode specifications.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-07-24T21:56:05.806Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-04T17:26:17.098Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/CMakeLists.txt:167-170
Timestamp: 2025-08-04T17:26:17.098Z
Learning: In CLP's CMake configuration for BZip2, Bill-hbrhbr prefers using HINTS parameter for path-based resolution rather than version pinning in find_package(). The primary concern is ensuring the task-installed BZip2 is found instead of system copies, not enforcing specific versions.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-07-01T14:51:19.172Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
📚 Learning: 2025-08-25T07:27:19.449Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1262
File: docs/src/dev-guide/components-core/index.md:50-50
Timestamp: 2025-08-25T07:27:19.449Z
Learning: In the CLP project, nlohmann_json version 3.11.3 is installed through their dependency management process, but they don't enforce version constraints in CMake find_package calls - they control versions through installation rather than CMake version pinning.
Applied to files:
components/core/src/clp_s/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: rust-checks
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (3)
components/core/src/clp_s/CMakeLists.txt (2)
1-31: CMake options structure looks solid.The use of
cmake_dependent_optionto gate exclusion and runtime linking flags is a clean approach for reducing the scope of conditional compilation and linking.
36-36: Verify that thetimestamp_parsersubdirectory exists.Line 36 adds a new subdirectory to the build, but this file alone does not show whether the directory and its CMakeLists.txt have been added to the repository. Please confirm that
components/core/src/clp_s/timestamp_parser/exists with a proper CMakeLists.txt.taskfile.yaml (1)
229-240: Static build outputs are correctly scoped to separate build directory.The
core-static-buildtask properly references{{.G_CORE_STATIC_COMPONENT_BUILD_DIR}}in the generates section and limits targets toclp-s, which aligns with the simplified static executable purpose.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
taskfile.yaml (1)
82-86: 🧹 Nitpick | 🔵 TrivialConsider adding a
clean-core-statictask for consistency.The existing
clean-coretask only removes the dynamic build directory. For developer ergonomics, consider adding a parallel task to clean the static build:♻️ Suggested addition
clean-core-static: cmds: - task: "utils:cmake:clean" vars: BUILD_DIR: "{{.G_CORE_STATIC_COMPONENT_BUILD_DIR}}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfile.yaml` around lines 82 - 86, Add a new task named clean-core-static mirroring the existing clean-core task: create a sibling task symbol clean-core-static that calls the same utils:cmake:clean task but sets the BUILD_DIR var to "{{.G_CORE_STATIC_COMPONENT_BUILD_DIR}}"; place it alongside clean-core for consistency and developer ergonomics so both dynamic (clean-core) and static (clean-core-static) build directories are cleaned the same way.
♻️ Duplicate comments (1)
components/core/src/clp_s/InputConfig.cpp (1)
197-225: 🧹 Nitpick | 🔵 TrivialConsider aligning error handling with "try_" naming convention.
The function
try_create_network_readerthrowsstd::runtime_errorwhenCLP_S_EXCLUDE_LIBCURLis defined, but the "try_" prefix conventionally suggests graceful failure with anullptrreturn (astry_create_file_readerdoes at lines 151-158).Since the team plans to add a dedicated exception type in
ystdlib-cppfor build-time feature exclusions, this can be revisited then. For now, ensure callers are prepared to handle the exception.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/core/src/clp_s/InputConfig.cpp` around lines 197 - 225, The function try_create_network_reader currently throws std::runtime_error under the CLP_S_EXCLUDE_LIBCURL branch; change it to follow the "try_" convention by returning nullptr instead of throwing so callers can handle failure gracefully (modify the CLP_S_EXCLUDE_LIBCURL arm of try_create_network_reader to return nullptr). Keep the rest of the function and error handling (including try_create_file_reader behavior) unchanged; when a dedicated build-exclusion exception is added later you can revisit to throw that specific type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@taskfiles/deps/main.yaml`:
- Around line 25-32: Remove the unresolved git conflict markers and merge the
two blocks so both variables exist: retain G_ZSTD_LIB_NAME and
G_ZSTD_INSTALL_PREFIX (the zstd entries) and also keep G_YSTDLIB_LIB_NAME;
delete the lines starting with <<<<<<<, =======, and >>>>>>> and ensure the
resulting YAML defines G_ZSTD_LIB_NAME, G_ZSTD_INSTALL_PREFIX, and
G_YSTDLIB_LIB_NAME with proper indentation and quoting so the file is valid
YAML.
---
Outside diff comments:
In `@taskfile.yaml`:
- Around line 82-86: Add a new task named clean-core-static mirroring the
existing clean-core task: create a sibling task symbol clean-core-static that
calls the same utils:cmake:clean task but sets the BUILD_DIR var to
"{{.G_CORE_STATIC_COMPONENT_BUILD_DIR}}"; place it alongside clean-core for
consistency and developer ergonomics so both dynamic (clean-core) and static
(clean-core-static) build directories are cleaned the same way.
---
Duplicate comments:
In `@components/core/src/clp_s/InputConfig.cpp`:
- Around line 197-225: The function try_create_network_reader currently throws
std::runtime_error under the CLP_S_EXCLUDE_LIBCURL branch; change it to follow
the "try_" convention by returning nullptr instead of throwing so callers can
handle failure gracefully (modify the CLP_S_EXCLUDE_LIBCURL arm of
try_create_network_reader to return nullptr). Keep the rest of the function and
error handling (including try_create_file_reader behavior) unchanged; when a
dedicated build-exclusion exception is added later you can revisit to throw that
specific type.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
components/core/src/clp_s/CMakeLists.txtcomponents/core/src/clp_s/InputConfig.cppcomponents/core/src/clp_s/JsonParser.cppcomponents/core/src/clp_s/clp-s.cpptaskfile.yamltaskfiles/deps/main.yaml
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Bill-hbrhbr
changed the title
glibc and libm to improve portability; Update mongocxx to link against statically built zstd.CLP_S_DISABLE_CURL build option to exclude libcurl/libcrypto paths for Emscripten compatibility.
Bill-hbrhbr
changed the title
CLP_S_DISABLE_CURL build option to exclude libcurl/libcrypto paths for Emscripten compatibility.CLP_S_DISABLE_CURL build option to exclude libcurl/libcrypto paths for WebAssembly compatibility.
Bill-hbrhbr
changed the title
CLP_S_DISABLE_CURL build option to exclude libcurl/libcrypto paths for WebAssembly compatibility.CLP_S_DISABLE_CURL build option to exclude libcurl/libcrypto paths for WebAssembly compatibility; Improve Curl error handling utilities.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a compile-time option,
CLP_S_DISABLE_CURL, to cleanly build clp-s without curl-dependent code paths.The primary goal is Emscripten compatibility. Since
USE_CURLis no longer supported in modern emsdk versions and building curl for wasm is unnecessary for our current browser workflow, we decouple curl from clp-s so wasm builds can succeed without network stack dependencies.Even if network-fetch functionality is needed later, it's generally recommended to implement it at the clp-ffi-js/Emscripten boundary (JS + wasm integration layer), rather than using curl-based networking inside C++ code paths.
Curl libraries have also historically been a recurring source of cross-toolchain build and link issues, so this change is useful beyond wasm and can be reused whenever upstream or downstream projects need a curl-free clp-s build.
With
CLP_S_DISABLE_CURLenabled, all code that depends onCURL_LIBRARIESis conditionally excluded. This disables network-based input paths in clp-s, while preserving core archive functionality needed by downstream clp-ffi-js.Result:
Checklist
breaking change.
Validation performed
clp-sin manylinux and verifies the exe works on the following platforms:Summary by CodeRabbit
New Features
Changes